Fix #5290: "Cannot use [] for reading." false positives#5059
Fix #5290: "Cannot use [] for reading." false positives#5059VincentLanglet merged 2 commits intophpstan:2.1.xfrom
Conversation
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
What about
function set(?bool &$value): void {
$value = true;
}
$array = [];
set($array[1][]);
var_dump($array);
?
|
Done. The nested case |
|
Already retrieved those results too — all 13,926 tests pass. Both |
|
I have never seen code like this in the wild. the original example feels theoretical. not sure its worth complicating the codebase for such a edge-case. not sure its worth spending our time on such a problem |
- Enter expression assign context for ArrayDimFetch with null dim when passed as a by-reference argument in NodeScopeResolver::processArgs() - Exit expression assign after processing to prevent scope leakage - Add regression test in tests/PHPStan/Rules/Arrays/data/bug-5290.php - Uncomment previously disabled test case for by-ref closure in existing test data Closes phpstan/phpstan#5290
Address review comment: test that `$array[1][]` passed to a by-reference parameter also does not produce a false positive. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
phpstan-bot
force-pushed
the
create-pull-request/patch-8uta713
branch
from
046172a to
21e150e
Compare
|
That was the earlier background |
The fix added doesn't seems that complicated. Also, the fact there is already in the codebase prove it was a wanted behavior.
Which tests do you need ? |
staabm
left a comment
staabm
left a comment
There was a problem hiding this comment.
ok - I did not see the pre-existing coverage for this feature.
feel free to merge.
|
Everything is green: both static analysis and tests pass. The PR is fully approved and ready to merge by a |
3 participants
Summary
PHPStan incorrectly reported "Cannot use [] for reading." when
$array[]was passed as an argument to a function/method parameter declared as by-reference. This is a valid PHP pattern —$array[]in a by-reference context creates a new array element and passes the reference to it, rather than reading.Changes
src/Analyser/NodeScopeResolver.phpinprocessArgs(): when processing a by-reference argument that is anArrayDimFetchwith null dim ($array[]), enter expression assign context before processing and exit it afterward. This tells theOffsetAccessWithoutDimForReadingRulethat the expression is in a write context.tests/PHPStan/Rules/Arrays/data/bug-5290.phpcovering named functions, immediately invoked closures with by-ref params, and method calls.tests/PHPStan/Rules/Arrays/data/offset-access-without-dim-for-reading.php(line 18:(function (&$ref) {})($array[])which was marked as "Should work but doesn't").Root cause
The
OffsetAccessWithoutDimForReadingRulechecks whether anArrayDimFetchwith null dim is in an expression assign context via$scope->isInExpressionAssign(). However,NodeScopeResolver::processArgs()did not enter expression assign context when processing by-reference arguments, so$array[]passed to a&$paramwas treated as a read rather than a write.The fix is targeted to only affect
ArrayDimFetchnodes with null dim (the$array[]syntax), avoiding broader side effects on other rules likeDefinedVariableRuleorUnusedPrivatePropertyRulethat also checkisInExpressionAssign.Test
Added
tests/PHPStan/Rules/Arrays/data/bug-5290.phpwith three scenarios that all previously produced false positives:set($array[])— named function with&$valueparameter(function (&$ref) {})($array[])— immediately invoked closure with by-ref parameter$foo->bar($array[])— method call with&$valueparameterAll three expect no errors.
Fixes phpstan/phpstan#5290